Skip to content

fix(auto-1m-guard): emit the advisory once per process, not per request - #355

Open
codeslake wants to merge 16 commits into
cnighswonger:mainfrom
codeslake:fix/1m-guard-advise-once
Open

fix(auto-1m-guard): emit the advisory once per process, not per request#355
codeslake wants to merge 16 commits into
cnighswonger:mainfrom
codeslake:fix/1m-guard-advise-once

Conversation

@codeslake

@codeslake codeslake commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

An advisory is advice, and advice does not need repeating

auto-1m-guard writes one stderr line when it sees context-1m-2025-08-07 on an outbound request. The wording is fixed for the mode the proxy runs in, so every copy after the first carried no information — and on a long-lived proxy it crowded everything else off stderr. Measured on one host: 55,685 lines, ~9.8 MiB, about 0.23 per second.

Latched to the first detection. Per-request detection is unaffected and still observable: the _auto1mGuard annotation is written on every detected request, spread into the per-session JSON, and read back by the statusline. The latch sits below the annotation, and a case pins that ordering.

The latch has to outlive a module instance

A module-scoped let is not once per process. loadExtensions cache-busts every import (+ "?t=" + cacheBuster), so the module is re-evaluated inside one process on every extension reload, and the latch re-arms with it. Measured through the real loadExtensions:

module-scoped latch:  one load, 5 requests  -> 1 advisory
                      two loads, 1 request each -> 2 advisories
globalThis latch:     two loads, 1 request each -> 1 advisory

The rate that fixes is once per extension reload — bounded by how often someone saves a file under proxy/extensions/, not by traffic. An earlier revision of this description said "at request rate"; that was wrong and the measurement above is what replaced it.

Symbol.for, not Symbol(): the registry is what makes every re-evaluated copy reach the same object, and mutating it to Symbol() kills the case.

Also here

request-capture declares routes: ["messages"] explicitly. runOnRequest defaults to exactly that, so the value changes nothing — what it changes is who can see it, since the SCOPE note at the top of that file reasons about the default and an inherited one is invisible to anyone widening the corpus from the export. Both other extensions relying on the same default spell it out.

That file's scope comment was also wrong about which half does the work: the outer half is the pipeline's (no routes declared → runOnRequest skips the hook for every other tagged route), and the body gate scopes an untagged caller. The case now drives the outer half through runOnRequest instead of calling onRequest directly, where it could not observe the real mechanism.

Verification

34/34 across the two touched test files; whole suite green on the rebuilt integration branch.

reverted dies
latch back to a module-scoped let two module instances in one process wrote 2 advisories
Symbol.forSymbol same case
routes widened to ["messages","bootstrap"] the bootstrap route reached the capture hook
the latch removed entirely both advisory cases

10 files changed, +140 / −12, including README, CHANGELOG, the directive, and the two translated READMEs — which still described the pre-latch behaviour and contradicted all three English documents.

One correction carried in the diff

classify() in proxy-held-port.test.mjs assumed a body. node --test runs files concurrently in CI and alone locally, and freePort() releases a port before its caller binds it — so a neighbouring file can hold the number this one just drew, and its 200 arrives where a body was expected. The file was 34/34 locally while test (22) was red; that difference was the finding. A non-string is not an ERR: body and is not an outage either.

codeslake and others added 10 commits August 24, 2026 11:03
The advisory names an env var to set. Its text never varies, so writing it on
every request carrying the 1m beta token tells a reader nothing the first line
did not, and it drowns everything else the proxy has to say.

Measured on a holder with 66h of uptime: its stderr log held 55,758 lines, of
which 55,685 were this one line (99.87%), 10 MB and still growing at roughly
3.6 MB/day. The same file contained zero error lines. A log that is 99.87% one
advisory is not an error log anyone can read.

Latched with a module-level flag, following the shape already used in
cc-version-normalize (_firstFireLogged / __resetFirstFireForTests), with the
matching test seam.

The test resets that latch before counting, and it must: ten earlier cases in
the same file already call onRequest, so without the reset the count reads 0
rather than 1 — indistinguishable from a correct single write, and green for
the wrong reason. Both halves are mutation-checked: removing the latch fails
"written 5x", removing the reset fails "written 0x".

Co-Authored-By: Claude <noreply@anthropic.com>
Complexity pass cut three comment blocks; the correctness pass that followed
found one of the cuts had made the file worse, so that one is reverted here
rather than shipped.

Kept cut: the test carried a three-line block restating the extension's own
comment and quoting measurements from a single run. The commit that introduced
the latch already holds those numbers, and a test file is read every time
someone touches it.

Reverted cut: the seam's comment is back to "Test seam — for unit tests that
want to clear the once-per-process latch". Shortening it to describe the body
dropped the only thing not derivable from the code — why a production module
exports this at all — which is the question the next reviewer asks. The
restored wording is also byte-identical to the same seam in
cc-version-normalize, which is where this pattern comes from.

Dropped a hard count ("Ten earlier tests") from the test comment: accurate
today, silently wrong the moment anyone adds a case, and the invariant does
not need the number.

Co-Authored-By: Claude <noreply@anthropic.com>
The extension's own header claimed "the proxy is the only component that
sees every request byte-for-byte" — 259 lines above the gate that
contradicts it. `onRequest` selects on the request BODY carrying a
`messages` array, so it records `/v1/messages` and nothing else.

That ordering is what makes it worse than a misleading name. A reader who
does the right thing and opens the source hits the universal claim first
and stops, so being careful returns a false confirmation.

**Which route the gate actually drops.** Only two paths reach the pipeline
at all: `/v1/messages` and `/api/claude_cli/bootstrap` (`server.mjs:707`).
Everything else — worker events, RC credentials, OAuth — is relayed by
`handlePassthrough`, which parses nothing, so it never reaches this hook
and is not the gate's doing. The gate's live customer is bootstrap, whose
bodies carry no `messages` array. A first draft of this comment said a
worker-events POST "returns there", which named a mechanism the routing
does not have — the same defect this commit exists to fix, in the
replacement.

Nothing pinned the scope either. The gate could be widened or dropped and
no test would say so, leaving the header as the only statement of what the
corpus covers. The new case asserts a non-Messages body writes nothing
while capture is ENABLED, with a premise that a Messages body still
writes, so it cannot pass because capture was simply off. Widening the
gate to `!ctx.body` fails it and fails nothing else.

Each ctx in that case carries its own session id. The boot record is
tracked per CAPTURE KEY, derived from the session id — not once per
process — so reusing a sibling's id burns the record that sibling asserts
on. A first draft did exactly that and was "fixed" by placing the case
last, which is a comment where an invariant belongs: nothing stops the
next capture-writing case being appended above the sibling. With distinct
ids the case passes in any position, measured first and last.

The directive and the replay/cache-sim tools the header cited are not in
the tree and never were, on any ref. Marked as such rather than re-flowed
unchanged, since a commit about comment truth should not re-affirm a
dangling pointer.

No README change: the extension has no README presence upstream, and
adding its first documentation is different work from correcting a claim
that is wrong.

10/10 in test/request-capture.test.mjs.

Co-Authored-By: Claude <noreply@anthropic.com>
The early return sits below the header strip and below the ctx.meta
annotation, and nothing said so. Hoisting it three lines up is the obvious
"collapse the early exits" edit, and it silently turns the guard off after
the first request of the proxy's life: cache-telemetry spreads that
annotation into every request's session JSON and the statusline reads
auto_1m_detected back out of it, so in strip mode the token stops being
removed from the wire and the guard goes invisible in telemetry, with the
suite still green.

Mutation-checked. With the latch hoisted above the annotation the new assert
fails "request 1 lost its annotation" (expected true, actual undefined).
Three earlier cases in the file fail under that mutation too, but only by
accident of file order and shared module state, and their message
("Cannot read properties of undefined") points nowhere near the cause.

Also trims the comment over the latch to the one clause the code does not
already say.

For the record, the latch's unit is the module instance, not the process:
loadExtensions cache-busts its import on every call, so an extension reload
re-arms the advisory instead of silencing it for the process lifetime.
Measured by importing the module twice under two query strings and counting
the lines: 1 after two requests on one instance, 2 after the re-import.

Co-Authored-By: Claude <noreply@anthropic.com>
A complexity pass over the branch diff. No behaviour change; the assertions
are the same ones, restructured.

request-capture.mjs: the SCOPE block spent four of its six lines on routes
that never reach this hook, and a further two announcing that a directive
and two tools are absent from the tree. Deleting the dangling pointer is
what makes the header true; a note about a file that does not exist is not
something a reader can act on. The gate keeps one statement of what it
drops, in the file header, not a shouting duplicate three lines above the
predicate that reads itself.

test/request-capture.test.mjs: an eight-line preamble, three lines of which
argued that the case should exist. The case name is that argument.

test/proxy-auto-1m-guard.test.mjs: an array collected across one loop only
to be re-walked by a second. The annotation assert moves into the loop that
makes the requests. Mutation-checked after the move: with the latch hoisted
above the ctx.meta write the case still fails "request 1 lost its
annotation" (expected true, actual undefined), the same signal and the same
message as before the restructure.

34/34 in the two touched files.

Co-Authored-By: Claude <noreply@anthropic.com>
Two claims on this branch described behaviour the code does not have.

request-capture's SCOPE comment said the body gate is what drops
/api/claude_cli/bootstrap. It is not. The extension declares no `routes`,
so the pipeline's route filter defaults to ["messages"] and never calls the
hook for a bootstrap-tagged request at all. Measured: driving runOnRequest
with route "messages" reaches the hook, route "bootstrap" does not. The
gate is what scopes an UNTAGGED caller, which appliesToRoute admits on
purpose for embedders and legacy tests.

That is the third draft of this comment to name a routing mechanism that
does not exist, and the second to survive a review pass, so the case now
pins the half that does the work: a MESSAGES body on the bootstrap route,
driven through runOnRequest so the body gate cannot be what drops it.
Mutation-checked — declaring routes ["messages", "bootstrap"] fails it with
"the bootstrap route reached the capture hook" and fails nothing else
(9 pass / 1 fail).

The advisory latch is per MODULE INSTANCE, not per process. loadExtensions
cache-busts its imports, so a reload re-arms it — which is the behaviour
you want, and the same boundary this repo already documents twice
elsewhere, once in the other file this branch touches. Measured: 1 line
after two requests on one instance, 2 after a re-import. The seam comment
and the case name say so now, and the directive's warn row carries the
frequency it was silent about.

Also: the advisory text does vary, by one clause, between warn and strip.
"The advice never changes" is the true form of that claim.

34/34 in the two touched test files.

Co-Authored-By: Claude <noreply@anthropic.com>
Complexity pass over the second round: three comment blocks carried their
facts in one more line than they need, and the premise assertion said
"not empty" as notDeepEqual against a literal empty array across three
lines. assert.ok on the length is one line and states the intent.
Mutation-checked after the rewrite: forcing isEnabled false still fails it
with "premise: capture is on, so a Messages body must write".

The README's mode table described warn as emitting a stderr line, which an
operator reads as one per detection. It is now latched, so the table says
so. The CHANGELOG entry is left alone: it records what shipped at the time
and is not a live description of behaviour.

34/34 in the two touched test files; 1948 pass / 0 fail across the suite
before this comment-only change.

Co-Authored-By: Claude <noreply@anthropic.com>
The mode summary at the top of auto-1m-guard read "stash annotation +
stderr line", 96 lines above the latch that stopped it being one per
request. That is the same defect, in the same shape, as the header this
branch corrects in request-capture: a reader who opens the source hits the
summary first and stops. It says "latched stderr line" now.

Dropped the claim that the advisory's text never varies. The ADVICE
constant does not, but the line written below it appends a clause in strip
mode, and the reason to latch does not need the claim: a repeat carries
nothing the first line did not.

Two test-hygiene fixes, both measured:

- the case left the latch set, so its position in the file was an unguarded
  invariant. Appending a probe case that expects the advisory: 1 with the
  reset in the finally, "appended case saw 0 advisories" without it. The
  probe was temporary; the reset is what stays.
- the stderr stub returned true for every write, so any diagnostic emitted
  while it was installed vanished. It forwards non-matching writes now:
  probed, a line written inside the stub reaches the real stream while the
  advisory is still counted.

The session ids in the capture case buy failure isolation, not what the
comment claimed: neither negative call writes under unmutated code, so
nothing is "burned" there. _bootWrittenFor is module-scoped, so what
distinct ids actually prevent is a MUTATION's write landing on a sibling
case's boot record.

CHANGELOG gains an Unreleased entry: the advisory is user-visible output
and an operator who greps for it after upgrading gets one line where they
used to get thousands.

NOT taken: the review called "Order 60 — after bootstrap-defense (45)"
moot, on the grounds that bootstrap-defense is routed to bootstrap and this
hook never sees that route. Measured through runOnRequest with three
metas: route "messages" runs request-capture alone, "bootstrap" runs
bootstrap-defense alone, and an UNTAGGED ctx runs BOTH, in order.
appliesToRoute returns true for a missing route, so the ordering governs
exactly the untagged caller the SCOPE block above it exists to describe.

34/34 in the two touched test files.

Co-Authored-By: Claude <noreply@anthropic.com>
Its sibling two asserts down says its piece in one line; this one spent
two on a clause the shorter form already carries.

NOT taken, and it is the largest cut on the table: the env save/restore
dance in this file is now on its third copy, and one helper would take
about twelve lines out of the file. Two of those three copies belong to
tests this branch did not write, and both review rounds have already
flagged this PR for carrying a second change its title does not name.
Adding an unrelated test refactor to it makes that worse, not better.

34/34 in the two touched test files.

Co-Authored-By: Claude <noreply@anthropic.com>
runOnRequest defaults `routes` to `["messages"]`, so the value changes
nothing. What it changes is who can see it: the SCOPE note at the top of this
file reasons about that default, and an inherited one is invisible to anyone
widening the corpus from the export.

Both other extensions that rely on the same default spell it out
(jsonl-session-mirror, image-retry-circuit-breaker), so this is the file
matching the two beside it rather than a new convention.

mutation: widening it to ["messages", "bootstrap"] fails
"the bootstrap route reached the capture hook"; 10/10 restored.
GREEN: 34/34 across the two touched files

Co-Authored-By: Claude <noreply@anthropic.com>

@vsits-codex-review-agent vsits-codex-review-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #355 auto-1m-guard advisory latch

Date: 2026-08-26
Reviewed: PR #355 at c8677fdeb39b4afb57179a294b0f2225f453b354, base 78ca94837129636de543df412edcea71fe0da6c8
Round: 1
Label applied: changes-requested

What Is Correct

  • [Read] proxy/extensions/auto-1m-guard.mjs:81 stores the latch in a module-scoped let _advised = false, and proxy/extensions/auto-1m-guard.mjs:106-114 writes the per-request _auto1mGuard annotation before checking the latch. That preserves per-request telemetry while suppressing repeated advisory lines inside one module instance.
  • [Measured] node --version && node --test test/proxy-auto-1m-guard.test.mjs test/request-capture.test.mjs on node v24.11.1 passed 34/34.
  • [Measured] A same-process cold-start concurrency probe (Promise.all over 50 ext.onRequest() calls after __resetAdvisedForTests()) produced {"concurrentCalls":50,"advisoryLines":1}. Because the code sets _advised = true synchronously before any await point, I do not see a same-thread Node event-loop race for duplicate advisory writes.
  • [Measured] Two fresh subprocesses each produced one advisory after two requests: {"firstSubprocess":"1","secondSubprocess":"1","stderr":""}. The latch is not machine-wide.
  • [Read] The new reset seam in proxy/extensions/auto-1m-guard.mjs:124-126 is explicit, and the new test resets before and after the count in test/proxy-auto-1m-guard.test.mjs:196-218, so existing file-order/test-fixture state does not hide a spent latch in that case.
  • [Read] The request-capture side change declares routes: ["messages"] in proxy/extensions/request-capture.mjs:261-267, matching the pipeline default at proxy/pipeline.mjs:96-99; it is a documentation/pinning no-op for route admission.

Blockers

  1. [Measured] The advisory is not once per process under the proxy's own extension reload path; it is once per module instance. proxy/extensions/auto-1m-guard.mjs:81 is module state, and proxy/pipeline.mjs:30-40 cache-busts every loadExtensions() import with a new query string, which re-evaluates that module in the same Node process. I measured the boundary directly:

    • Same specifier repeated import: {"sameSpecifierSameModule":true,"advisoryLines":1}
    • Cache-busted imports in the same process: {"cacheBustedSameModule":false,"advisoryLines":2}
    • Two loadExtensions() calls in the same process: {"advisoryLinesAfterReload":2}

    That contradicts the commissioned contract and PR title, which are "once per process", and leaves the noise budget dependent on whether the extension graph reloads. The directive text now admits the weaker behavior at docs/directives/proxy-auto-1m-guard.md:82, and the changelog admits it at CHANGELOG.md:9, but the requested change was a process-lifetime latch. Anchor the latch to process-durable state, for example a Symbol.for(...) property on globalThis, or narrow the PR contract/title/review request to "once per module instance" if that is the intended behavior.

What Needs Attention

  • [Read] README.md:618 says the stderr line is "latched to the first detection" without naming the module-instance reload boundary that the directive and changelog now name. If the final contract stays module-instance scoped, the README should say that too so operators do not read it as process lifetime.
  • [Measured] CI for head c8677fdeb39b4afb57179a294b0f2225f453b354 was green when reviewed: test (18), test (20), test (22), GitGuardian, and Snyk all reported success.

Bloat / Non-Functional

  • [Read] Production diff size is small for the claimed defect: roughly 31 production/docs lines in the two touched extension files plus docs, with 71 test lines across the two touched test files. New surface area is one test-only export (__resetAdvisedForTests) and no new env vars, config keys, or on-disk paths. No bloat finding.

Recommendations

  • If process lifetime is the contract, use a process-global latch, e.g. const state = globalThis[Symbol.for("cache-fix.auto-1m-guard")] ??= { advised: false }, and assert it through loadExtensions() reloads, not only repeated calls on a single imported module.
  • Keep the existing per-request annotation assertions. They pin the important placement of the latch below the telemetry write.

Bottom Line

Revise before merge. The per-module latch fixes repeated requests through one imported extension instance, but the measured loadExtensions() path re-arms the advisory inside the same process, so the core once-per-process claim is not satisfied.

— Codex, cross-LLM review, round 1

@vsits-codex-review-agent vsits-codex-review-agent Bot added changes-requested Blocking review findings are outstanding reviewed-by-codex-agent Directive/spec reviewed by Codex — no blocking findings labels Aug 26, 2026
codeslake and others added 2 commits August 26, 2026 10:17
`loadExtensions` cache-busts every import, so a module-scoped `let` is re-armed
on each extension reload and the advisory this exists to silence comes back at
request rate wherever hot reload is on. Measured: two cache-busted imports in
ONE process wrote two advisory lines.

The latch now lives on a `Symbol.for`-keyed object on `globalThis`. The
registry is the load-bearing half: `Symbol()` would mint a fresh key per
re-evaluated copy and change nothing.

Title, README, CHANGELOG and the directive said "process"; an earlier round
lowered three of them to "module instance" to match the code rather than
fixing the code. They now say process again, and the code earns it.

The stderr sampler is one helper instead of two copies. They had already
diverged -- one swallowed every stderr line, the other forwarded what it was
not sampling.

RED: two module instances in one process wrote 2 advisories
GREEN: 35/35 across the two touched test files
mutation: module-scoped `let` -> the new case dies; `Symbol.for` -> `Symbol`
  -> the new case dies; restored -> 25/0

Co-Authored-By: Claude <noreply@anthropic.com>
Review measured the pre-fix behaviour rather than taking the claim: one
`loadExtensions()` call plus five detected requests emitted ONE advisory, and
two `loadExtensions()` calls emitted two. So the old latch re-armed once per
extension RELOAD -- bounded by how often someone saves a file under
proxy/extensions/, not by traffic.

That wording was the sole stated justification for the change and it sat in the
shipped CHANGELOG. The change is still right; the reason now says what was
measured.

Also here: the two translated READMEs still described the pre-latch behaviour,
so they contradicted all three English documents. Pre-existing drift the latch
work never reached, corrected while the section is open.

The extension comment now points at `process.env`, which is how server.mjs and
request-capture.mjs keep the same kind of reload-durable state, so a reader
meets the second idiom deliberately instead of finding it.

GREEN: 35/35 across the two touched test files

Co-Authored-By: Claude <noreply@anthropic.com>
… number

`test (22)` has been red on this branch while 18 and 20 pass, and the file is
34/34 locally. That difference IS the finding: `node --test` runs files
concurrently in CI and alone here, and `freePort()` releases a port before its
caller binds it, so a neighbouring file can hold the number this one just drew.
Its 200 arrives where a body was expected and `classify()` threw
`body.startsWith is not a function` -- out of the helper whose whole job is to
answer "is this an outage".

A non-string is not an ERR: body and is not an outage. Guarded at the boundary
rather than at the eleven call sites that reach it.

This is the smaller half of what `3d50a9f` does on fix/reap-fingerprint-records,
which also replaces the readiness predicate so a neighbour's 200 is not read as
ready. Cherry-picking it here conflicts in four hunks -- the two branches have
diverged on this file -- and resolving that would mint a third version of it,
so this takes only the part that is unambiguous and identical in intent.

Verified: 200 -> null, "ERR:ECONNREFUSED" -> refused, null/undefined -> null.
GREEN: 34/34 locally; CI is the experiment this push runs.

Co-Authored-By: Claude <noreply@anthropic.com>
codeslake added a commit to codeslake/claude-code-cache-fix that referenced this pull request Aug 26, 2026
Resolved test/proxy-held-port.test.mjs: both sides add the same type guard to
classify() and differ only in wording. Kept cnighswonger#355's, because cnighswonger#345's says
"health() replaced the probe that resolved a bare statusCode" and health()
itself resolves a bare 200 -- refuted in review. Behaviour is identical.

Co-Authored-By: Claude <noreply@anthropic.com>
`classify` keys on an `ERR:` prefix. Three of its four callers test that
prefix before calling; the fourth's probe resolved a bare status NUMBER
and a bare errno STRING, so every classification came back null and the
refusal count in the case named for counting refusals was provably zero:

    cut      = [503,503,ECONNREFUSED,ECONNRESET,ETIMEDOUT,404]
    classify = [null,null,null,null,null,null]
    refused  = []            -> assert refused.length <= 2 : true

The TypeError that shape mismatch used to throw was the only signal the
two disagreed, and a `typeof body !== "string"` guard removed it while
leaving the mismatch. The probe now resolves `ERR:`-shaped strings like
its three siblings and the guard is back to the one clause it needs.

The same probe could not resolve on a socket timeout, which leaves the
hammer's `await` pending for good; handled now, and at 1s rather than 3s
because a handled timeout spends the readiness budget where an unhandled
one cost nothing. The budget moves 10s -> 25s to match this file's other
fixtures: at 3s against 10s the loop got three tries under whole-file
load and the stand-in had not come up.

Pinned as a contract rather than through the outage case, which bounds
refusals at <= 2 and cannot go red until three real ones arrive: two new
cases assert what classify returns for each ERR: shape a probe emits,
that a BARE errno is null, and that the gap relay's deliberate 503 is
still not an outage.

36 pass, 0 fail.

Co-Authored-By: Claude <noreply@anthropic.com>
@codeslake

Copy link
Copy Markdown
Contributor Author

Thanks — the module-instance boundary was the right call, and it is fixed at a
later head.

Reviewed: c8677fde. Current head: 5d2159f.

The latch is process-durable now, anchored exactly as you suggested:

// `Symbol.for`, not `Symbol()` -- the registry is what makes every
// cache-busted instance find the same object.
const _latch = (globalThis[Symbol.for("cache-fix.auto-1m-guard")] ??= { advised: false });

Measured with your own boundary probe — two cache-busted imports in one
process, each driven with a ctx carrying the 1M beta token:

head cacheBustedSameModule advisory lines
c8677fde (reviewed) false 2
5d2159f (head) false 1

The modules are still distinct objects — loadExtensions() cache-busts as
before — and the advisory is now emitted once for the process regardless.

The wording is back to the stronger contract rather than the weaker one the
earlier round had lowered it to. README.md:618 now reads "latched to the first
detection for the life of the process (the advice never changes; an extension
reload does not re-arm it)"
, and the directive and CHANGELOG say the same, so
the README no longer under-promises against the code.

node --test test/proxy-auto-1m-guard.test.mjs → 25/25 pass at head.

Could you re-review at 5d2159f?

🤖 Generated with Claude Code

codeslake added a commit to codeslake/claude-code-cache-fix that referenced this pull request Sep 3, 2026
…ger#356 merge left behind

conflict-shape.sh's additive resolution on the cnighswonger#356 merge kept both sides of
two hunks where cnighswonger#356 (cut from upstream, before cnighswonger#345 or cnighswonger#355 existed)
independently re-added content cnighswonger#345 had already added on this branch: a
second `if (typeof body !== "string") return null;` guard (with its own,
now-superseded comment) stacked dead beneath the one already in classify(),
and a second, byte-identical copy of the
"classify survives a probe that answers with a status code" test right after
it. Both were textually different insertions at the same conflict hunk (so
"additive" concatenated them) but semantically the same content twice.
Neither duplicate changed behaviour -- the second guard clause is
unreachable, and node:test does not refuse a duplicate case name -- so the
suite passed either way; kept once, as the recorded resolution for this file
already documents keeping ours' guard.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QFPGPPSYmx8NNGqbEsSpNc

Co-Authored-By: Claude <noreply@anthropic.com>
codeslake added a commit to codeslake/claude-code-cache-fix that referenced this pull request Sep 6, 2026
Conflict in test/proxy-held-port.test.mjs (classify()'s typeof guard):
matches rebuild-resolutions.md's alternative 2, block 1, standalone (block 2
of that entry does not arise in this build order since cnighswonger#345 has not merged
yet). Kept ours' guard (if (typeof body !== "string") return null;, carried
into this build via cnighswonger#356's cherry-pick of cnighswonger#345's 35ac847) and dropped theirs'
(cnighswonger#355, 5d2159f) removal of it.

Measured: re-applying theirs' hunk alone and running
'classify survives a probe that answers with a status code' reproduces the
exact TypeError the ledger records (body.startsWith is not a function). The
resolved file passes that case and the other two classify cases (3/3); the
parent commit passes them too, so nothing here is new.

Co-Authored-By: Claude <noreply@anthropic.com>
codeslake added a commit to codeslake/claude-code-cache-fix that referenced this pull request Sep 6, 2026
…to HEAD

Two judgement conflicts, both orthogonal-halves-of-one-block (same pattern
already recorded for cnighswonger#368/cnighswonger#369):

test/proc-helpers.mjs: cnighswonger#345 guards onPort(0) against selecting every proxy
child (CACHE_FIX_PROXY_PORT=0), cnighswonger#369 (already in this build) added
probeHealth/waitForHolder right after the same line. No shared subject; kept
both — theirs' guarded onPort(), ours' probeHealth/waitForHolder unchanged.

test/proxy-held-port.test.mjs, block in 'refuses nothing when the proxy under
it dies': cnighswonger#345 proposes swapping the local 'ok'-sentinel probe (cnighswonger#355's, ours)
for the shared health(port) helper it adds elsewhere in the file, which
resolves the number 200 rather than the string "ok". Three lines below this
hunk, 'const cut = seen.filter((c) => c !== "ok")' already depends on the
'ok' sentinel — taking theirs would silently make cut === seen (the exact
defect this file's classify()/probe rewrite exists to prevent). Kept ours
whole.

Also dropped two merge-additive duplicates the mechanical classifier cannot
see, same class as the recorded 'cnighswonger#356 duplicates' ledger entry: a
byte-identical second copy of 'classify survives a probe that answers with a
status code' (cnighswonger#345's own commit landing both directly and via cnighswonger#356's earlier
cherry-pick of it), and a second, differently-worded typeof guard cnighswonger#345 stacked
under the first.

Verified: node --test test/proxy-fingerprint-reap.test.mjs (7/7, including
'onPort(0) selects nothing, and still selects on a real port'), and
--test-name-pattern=classify in test/proxy-held-port.test.mjs (4/4, no
duplicate case).

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes-requested Blocking review findings are outstanding reviewed-by-codex-agent Directive/spec reviewed by Codex — no blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant